Skip to content

Make issue close safe to retry with an idempotency key - #332

Merged
wesm merged 16 commits into
kenn-io:mainfrom
naveenspark:feature/close-retry-safety
Sep 3, 2026
Merged

Make issue close safe to retry with an idempotency key#332
wesm merged 16 commits into
kenn-io:mainfrom
naveenspark:feature/close-retry-safety

Conversation

@naveenspark

@naveenspark naveenspark commented Sep 2, 2026 •
edited by wesm
Loading

Copy link
Copy Markdown
Contributor

Closing an issue can now be retried safely after a lost response. Send Idempotency-Key on the close request (kata close --idempotency-key, MCP idempotency_key) and a retry returns the original close instead of running a second one. If-Match (kata close --if-match, MCP revision) rejects a close of an issue that changed underneath you.

Before this, a client that timed out after the daemon committed had no safe move. Retrying gave a no-op with no event, and the CLI's follow-up --comment could post twice.

What a retry gets:

  • The same key with the same request returns the original receipt. A different body, actor, issue, or revision guard gets 409.
  • If someone else closed the issue first, or moved it before the write, the keyed request gets 409 rather than a silent no-op.
  • If the database committed but the response was lost, the daemon publishes the committed events once, then replies with the receipt.
  • A replayed receipt follows the same visibility rules as a fresh request: active project, correct route, and the caller's host scope.
  • Retry headers require retry_protocol: "close-v1" in the body. Old daemons reject the unknown field instead of silently ignoring the headers. API schema version is 0.15.0.

No schema change. Hook delivery stays in memory, so a daemon crash between commit and publish loses hook jobs for a close the same way it does for every other mutation.

Also: a comment idempotency key now belongs to one issue, not one project, and a comment retry survives a project move.

@roborev-ci

roborev-ci Bot commented Sep 2, 2026

Copy link
Copy Markdown

roborev: Combined Review (c5501ea)

Verdict: High-severity replay authorization and concurrency flaws remain, along with medium-severity retention, backup, and PostgreSQL privilege gaps.

High

  • internal/daemon/handlers_comments.go:52-79: UID-based idempotency lookup ignores the requested project and runs before active-project validation, allowing comment replay through foreign or archived project routes. Validate the route and issue’s current project before replay, permitting only supported moved-issue cases.

  • internal/daemon/handlers_comments.go:37-50: Short-ID and UID retries use different idempotency locks, so concurrent requests for the same issue/key can create duplicate comments. Canonicalize lock keys by issue UID.

  • internal/daemon/handlers_actions.go:68-90, internal/daemon/handlers_actions.go:430: Close replay returns receipt data without revalidating the route project and the issue’s current project authorization. An issue moved to an inaccessible or archived project can still be replayed successfully. Re-run active-visibility and host-scope authorization before publishing or returning replay responses.

Medium

  • internal/daemon/handlers_comments.go:52-65: Project-scoped retries fingerprint against the matched issue UID without ensuring it matches the requested issue, so reusing a key and body on another issue can return the first issue’s comment. Resolve the requested issue first and require UID equality.

  • internal/db/sqlitestore/close_event_deliveries.go:37-44, internal/db/pgstore/close_event_deliveries.go:37-44: Delivery rows are retained indefinitely while idempotency lookup expires after seven days. Reusing a key afterward can fail on the delivery table’s primary key. Safely expire old delivery rows or include a retention generation in the identity.

  • internal/db/sqlitestore/close_event_deliveries.go:14-44: Durable close-delivery state is absent from JSONL export/import, so restores can preserve close events while losing pending delivery batches. Export/import delivery rows, including claim and delivered state, or reconstruct pending batches during restore.

  • internal/db/pgstore/open.go:485-493: close_event_deliveries is missing from the canonical table-name registry, allowing split-role PostgreSQL privilege validation to pass without checking required access. Add it to canonicalTableNames and cover its runtime privileges.

Reviewers: 2 done | Synthesis: codex, 12s | Total: 10m8s

@roborev-ci

roborev-ci Bot commented Sep 3, 2026

Copy link
Copy Markdown

roborev: Combined Review (c182569)

Verdict: Medium-severity issues remain around idempotency, archival visibility, backup restoration, retention, and duplicate event delivery.

Medium

  • internal/daemon/handlers_comments.go:32-35
    Lowercase full ULIDs are accepted by resolution but fail uid.Valid, allowing retries after issue moves to miss the original idempotency event and create duplicate comments. Canonicalize references with shortid.Parse before lookup and locking.

  • internal/jsonl/storage_export.go:112-127, internal/db/sqlitestore/import_replay.go:339-397
    close_event_deliveries is omitted from JSONL export/import. Restoring a database with an undelivered close loses its delivery record, preventing hooks and live clients from receiving the event batch. Add export/replay support and restore in-flight claims as pending.

  • internal/daemon/handlers_actions.go:213-218, internal/db/sqlitestore/schema.sql:222-238
    Delivery rows are retained indefinitely while close idempotency keys expire after seven days. Reusing a key can fail on the permanent primary key and return an internal error. Expire completed delivery rows consistently while retaining pending deliveries.

  • internal/daemon/handlers_actions.go:365-383
    Events are broadcast and queued before delivery completion is persisted. A crash can cause republishing, while broadcasters and hook sinks do not deduplicate event UIDs. Add downstream idempotency or durable per-sink acknowledgements, and enforce/test the intended at-least-once semantics.

  • internal/daemon/handlers_comments.go:52-79; internal/daemon/handlers_actions.go:74-89, internal/daemon/handlers_actions.go:414-431
    Idempotent replay paths call IssueByID before checking archived-project visibility, allowing prior comments or close events to be replayed after archival instead of returning project_not_found. Verify the current project with activeProjectByID before returning replay responses or publishing replayed events.

Reviewers: 2 done | Synthesis: codex, 11s | Total: 17m11s

@wesm

wesm commented Sep 3, 2026

Copy link
Copy Markdown
Member

looking

@wesm wesm self-assigned this Sep 3, 2026
@wesm @claude
Replay keyed closes from the events table instead of a delivery table
a2ad38c 
A keyed close retry only needs the committed issue.closed receipt, which
the events table already stores under the idempotency key. Schema version
27 and the close_event_deliveries table existed to re-broadcast the event
batch after a daemon crashed between commit and publish. That gap exists
for every mutation today because the hook queue is in-memory, and SSE
clients already recover missed events through Last-Event-ID, so a
per-mutation delivery table and claim lease bought little for its cost.

The table also carried two defects: a key reused after the seven-day
lookup window collided with its permanent primary key and returned a
500, and the PostgreSQL table registry never listed it, so split-role
privilege validation skipped it.

The ambiguous-commit case still publishes exactly once. Both stores
return the attempted event batch alongside a commit error, and the
handler publishes that batch only when the receipt it finds is the same
event this attempt wrote. Replayed receipts now also check the caller's
host scope against the issue's current project, matching the comment
replay path, so a moved issue cannot leak state through the old route.

Generated with Claude Code
Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
@wesm wesm changed the title Make issue close safe to retry Make issue close safe to retry with an idempotency key Sep 3, 2026
@roborev-ci

roborev-ci Bot commented Sep 3, 2026

Copy link
Copy Markdown

roborev: Combined Review (a2ad38c)

Verdict: Changes have medium-severity correctness and authorization issues in idempotent replay handling.

Medium

  • Inconsistent idempotency keys for short IDs vs. UIDs — internal/daemon/handlers_comments.go:37-46
    The two reference forms can acquire different locks, allowing concurrent retries to create duplicate comments. Canonicalize both to the same issue UID/key before lookup and mutation.

  • Project scope is not validated before UID-based replay — internal/daemon/handlers_comments.go:52-75
    A request can replay a receipt for an issue UID under an unrelated project instead of returning not found. Validate that the requested project matches the receipt’s original project or the issue’s current project for legitimate move-retry cases.

  • Replay paths bypass archived-project visibility checks — internal/daemon/handlers_comments.go:65-79; internal/daemon/handlers_actions.go:350-364
    Comment and close retries use IssueByID, which includes archived projects, and can expose issue state or mutation receipts after archival. Verify the issue’s current project with activeProjectByID before replaying, while preserving soft-deleted-issue replay behavior.

Reviewers: 2 done | Synthesis: codex, 9s | Total: 11m31s

@wesm @claude
Scope comment retry keys to the issue and guard replayed receipts
56e0299 
A comment idempotency key now identifies one request to one issue. Before
this, a short-id request looked the key up by project while a ULID request
looked it up by issue, so the same key sent to a second issue in the same
project with the same body replayed the first issue's comment instead of
posting one, and the two ref forms took different locks for the same
retry. Every keyed comment now resolves to an issue UID first and locks,
looks up, and fingerprints under that UID. A retry with a full ULID still
survives a project move because it needs no route-scoped resolution.

Replaying a close or comment receipt exposes the issue's current state, so
the replay paths now apply the same visibility rules as a fresh request:
the route must be the project the receipt was written in or the project
the issue lives in now, that current project must not be archived, and it
must be inside the caller's host scope. Without this, a receipt could be
read through an unrelated project route or after its project was archived.

Generated with Claude Code
Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
@roborev-ci

roborev-ci Bot commented Sep 3, 2026

Copy link
Copy Markdown

roborev: Combined Review (56e0299)

Verdict: Two medium-severity issues remain in comment retry handling.

Medium

  • internal/daemon/handlers_comments.go:43-51 — Keyed comment retries using short or qualified references resolve against the current project before checking the idempotency receipt. After an issue moves, retries return 404 instead of replaying the committed comment. Resolve via a stable issue UID or retain receipt lookup for the original project/reference.

  • internal/daemon/handlers_comments.go:165-187 — Full-UID retries bypass activeIssueByRef; after soft deletion, replayComment uses IssueByID and returns the deleted issue and comment instead of the normal deleted-issue behavior. Reject replay when current.DeletedAt != nil.

Reviewers: 2 done | Synthesis: codex, 7s | Total: 17m34s

@wesm @claude
Replay short-id comment retries after a move and reject deleted issues
acf8a91 
A keyed comment retry that names the issue by short ID resolves inside
the route project. After the issue moves, that resolution fails and the
retry returned 404, so the caller could not recover its receipt and might
post the comment again in the new project. The receipt written in the
route project still names the issue, so the handler now uses it to find
the issue UID when the short ID is a suffix of that UID and any qualifier
names the route project. The locked, issue-scoped lookup then replays as
usual. A key alone cannot steer a retry to a different issue.

A full-ULID retry skips ordinary resolution, so it replayed the comment
and issue after the issue was soft-deleted. Replay now returns the same
not-found response a fresh comment on a deleted issue gets.

Generated with Claude Code
Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
@roborev-ci

roborev-ci Bot commented Sep 3, 2026

Copy link
Copy Markdown

roborev: Combined Review (acf8a91)

Verdict: Medium-severity event-delivery gap found in keyed close retries.

  • Medium — internal/daemon/handlers_actions.go:72-77, 249-256
    A keyed close retry can return a persisted receipt without republishing its event batch. If the original close commits but response or publishing is lost, retries may succeed while issue.closed and related audit events are never delivered to live SSE clients or hooks. Persist/claim event delivery, or reload and republish the committed batch with durable, idempotent coordination.

Reviewers: 2 done | Synthesis: codex, 6s | Total: 17m27s

@wesm

wesm commented Sep 3, 2026

Copy link
Copy Markdown
Member

Accepted as is

@wesm
wesm merged commit b6a66a5 into kenn-io:main Sep 3, 2026
26 of 27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@wesm wesm

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@naveenspark @wesm